Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow moderated secrets usage on PRs from forks #893

Merged
merged 2 commits into from
Nov 22, 2024
Merged

Allow moderated secrets usage on PRs from forks #893

merged 2 commits into from
Nov 22, 2024

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Nov 21, 2024

This PR implements the approach recommended by this article from Github's ‘Security Lab’ to allow the usage of secrets on PRs from forks of the message_ix repository.

  • Add a "receive" CI workflow.
    • Run on all PRs into main.
    • Fail if (a) the PR is from a fork and (b) it does not have a certain label, currently "safe to test".
  • Adjust the "pytest" CI workflow:
    • Add a workflow_run: event trigger. Invoke this trigger every time the "receive" workflow completes (success or failure).
    • Skip the main pytest and tutorials jobs if the "receive" workflow failed.
    • Invoke actions/checkout with the right options to use the fork PR branch.

With this merged, it would be up to some user with ‘write’ permissions on the current repo to give a first look-over of any PR from and then add the "safe to test" label. (The label could also be removed again at any time.)

How to review

Per the documentation:

This [workflow_run] event will only trigger a workflow run if the workflow file is on the default branch.

This means we will need to:

  • Merge this PR.
  • Create or update another PR (Add .util.sankey and tutorial #770 was suggested).
  • Confirm that the pytest workflow is invoked and runs successfully; if not, open a follow-up PR to adjust the workflow(s).

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅ N/A; CI changes only
  • Add, expand, or update documentation.
  • Update release notes.

@khaeru khaeru force-pushed the ci/fork branch 7 times, most recently from 4da98c2 to 89fb99e Compare November 21, 2024 15:21
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.6%. Comparing base (b0365d6) to head (0ffb789).
Report is 3 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #893   +/-   ##
=====================================
  Coverage   95.6%   95.6%           
=====================================
  Files         46      46           
  Lines       4337    4337           
=====================================
  Hits        4147    4147           
  Misses       190     190           

@khaeru khaeru force-pushed the ci/fork branch 3 times, most recently from 0425eb0 to 684dea9 Compare November 21, 2024 15:41
@khaeru khaeru self-assigned this Nov 21, 2024
@khaeru khaeru added the ci Continuous integration label Nov 21, 2024
Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks promising to me :)
(Rebasing on main.)

@khaeru khaeru merged commit 433868d into main Nov 22, 2024
27 checks passed
@khaeru khaeru deleted the ci/fork branch November 22, 2024 08:42
@khaeru
Copy link
Member Author

khaeru commented Nov 28, 2024

@glatterf42 FYI this appears not to have worked exactly as intended 🥲

When I opened #894:

  • This workflow run on main was triggered —which should not have happened, or it should have exited early because that is not a PR from a fork.
  • It failed, because this value
    repository: ${{ github.event.workflow_run.repository }}

    …appears to be a JSON structure rather than the desired string "iiasa/message_ix".

I'll make a note to come back and revisit this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants